Reject out-of-range literals when binding IN / NOT IN by jackylee-ch · Pull Request #3916 · apache/iceberg-python

jackylee-ch · 2026-09-07T03:25:01Z

Rationale for this change

SetPredicate.bind converts every literal to the field's type. A literal outside that range converts to an AboveMax/BelowMin sentinel that carries the clamped boundary value, so it starts matching rows there:

In("id", [1, 2**40]).bind(schema)     # matches id = 2147483647
NotIn("id", [1, 2**40]).bind(schema)  # excludes id = 2147483647

Keep the original literal instead of the sentinel. It holds a value the field can never take, so the bound set still round-trips to what the caller wrote, and value_set leaves that value out. Every BoundBooleanExpressionVisitor receives value_set, so that is the one place the range has to be taken into account.

Storing the sentinel is not an option: it is == and hash-equal to the boundary literal it clamps to, so {IntAboveMax(), LongLiteral(2147483647)} collapses to a single element and a boundary value the caller did write can be lost.

Are these changes tested?

Yes, in tests/expressions/test_evaluator.py and tests/io/test_pyarrow.py: the round trip through as_unbound, expression_evaluator, the inclusive and strict metrics evaluators, and an end-to-end scan including after a type promotion. 17 of the 22 cases fail without the change.

Are there any user-facing changes?

IN/NOT IN with a literal outside the column's range no longer matches or excludes rows at the range boundary. A NOT IN that reduces to a single literal takes the != push-down, which drops rows where the column is null; #3918 (draft) has the details.

`SetPredicate.bind` kept the result of `Literal.to(field_type)` for every literal. For a value outside the field's range that result is the `AboveMax`/`BelowMin` sentinel, whose value is the type's max/min, so the bound set held a literal the user never wrote. On an `int` column, `id in (1, 2**40)` matched rows where `id` equals 2147483647, and the `not in` form dropped them. `LiteralPredicate.bind` already folds these to `AlwaysTrue`/`AlwaysFalse`, and Java's `bindInOperation` filters them out of the set; do the same here. An empty set after filtering is already folded by `BoundIn`/`BoundNotIn`. Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The current set-building approach in SetPredicate.bind can de-duplicate a real boundary literal against an out-of-range sentinel and then drop it during filtering, changing semantics for inputs like [IntegerType.max, IntegerType.max + 1].

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adjusts expression binding in pyiceberg.expressions so IN / NOT IN predicates no longer “clamp” out-of-range literals into the bound set (via AboveMax/BelowMin), aligning behavior with other predicate types and preventing incorrect matches at type boundaries.

Changes:

Update SetPredicate.bind to drop AboveMax/BelowMin sentinels produced by Literal.to(field_type) when binding IN / NOT IN.
Add evaluator-level regression tests ensuring out-of-range IN / NOT IN no longer matches/excludes the clamped boundary values.
Add bind-form assertions verifying folding behavior when the filtered set becomes empty or singleton.

File summaries

File	Description
`pyiceberg/expressions/__init__.py`	Filters out-of-range literal sentinels during `IN` / `NOT IN` binding so they can’t match boundary values.
`tests/expressions/test_evaluator.py`	Adds regression tests for above-max / below-min literals in `IN` / `NOT IN` binding and evaluation.

Review details

Suppressed comments (1)

tests/expressions/test_evaluator.py:1935

Add the analogous boundary+out-of-range assertion for the lower bound case too (e.g., [IntegerType.min, IntegerType.min - 1]) to ensure binding never drops a user-provided IntegerType.min when an out-of-range literal is present.

    below_min = IntegerType.min - 1

    assert In("id", [1, below_min]).bind(schema) == EqualTo("id", 1).bind(schema)
    assert NotIn("id", [1, below_min]).bind(schema) == NotEqualTo("id", 1).bind(schema)
    assert In("id", [below_min]).bind(schema) == AlwaysFalse()

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        field_type = bound_term.ref().field.field_type
+        # Literals outside the field's range can never match, so drop them rather
+        # than keep the clamped AboveMax/BelowMin sentinel in the bound set
+        bound_literals = {lit.to(field_type) for lit in self.literals}
+        return self.as_bound(  # type: ignore
+            bound_term, {lit for lit in bound_literals if not isinstance(lit, (AboveMax, BelowMin))}
+        )


+def test_above_int_bounds_in() -> None:
+    schema = Schema(NestedField(1, "id", IntegerType(), required=False))
+    above_max = IntegerType.max + 1
+
+    assert In("id", [1, above_max]).bind(schema) == EqualTo("id", 1).bind(schema)
+    assert NotIn("id", [1, above_max]).bind(schema) == NotEqualTo("id", 1).bind(schema)
+    assert In("id", [above_max]).bind(schema) == AlwaysFalse()
+    assert NotIn("id", [above_max]).bind(schema) == AlwaysTrue()
+
+    # The clamped literal used to match the field's maximum
+    assert expression_evaluator(schema, In("id", [1, above_max]), True)(Record(IntegerType.max)) is False
+    assert expression_evaluator(schema, NotIn("id", [1, above_max]), True)(Record(IntegerType.max)) is True


Collecting the converted literals first let an AboveMax/BelowMin sentinel absorb a boundary value the user did write: the sentinel's value is the type's max/min, and Literal equality compares only the value, so `{IntAboveMax(), LongLiteral(2147483647)}` has one element. Filtering after that dropped both, turning `id in (2147483647, 2**40)` into AlwaysFalse. Filter inside the comprehension so a sentinel never enters the set, and add the boundary-plus-out-of-range case to the tests. Co-Authored-By: Claude Code <noreply@anthropic.com>

Fokko

Thanks @jackylee-ch for adding this. I believe you hit a edge case here. However, I don't like the idea of dropping these literals when binding:

For example, the following is unexpected for me:

lit = NotIn("id", [1, 2**40])
assert lit.bind(schema).as_unbound() == lit

Which would fail with the change suggested by this PR.

Instead, this should be handled by the evaluators. This is also where we handle the out of bounds case of the non-set operators. I'm curious what the current behavior is when feeding this into the evaluators, maybe we can start with some tests over there.

Keep null rows when NOT IN simplifies to NotEqualTo so Arrow scans agree with the expression evaluator. Cover bounds, metrics, and int-to-long schema evolution with evaluator and file scan regression tests. Generated-by: Codex

Pushing NotEqualTo down to Arrow drops rows where the column is null. That is a pre-existing bug, not one this change introduces: a single-literal NOT IN already folded to NotEqualTo before it. It changes the result of every `!=` row filter, so it belongs in its own change rather than here. Drop the null rows from the two scan tests so they no longer depend on it. `visit_not_in` needs no change, so a NOT IN that keeps two or more literals still keeps its nulls. Co-Authored-By: Claude Code <noreply@anthropic.com>

jackylee-ch · 2026-09-07T09:41:20Z

bindInOperation filters aboveMax/belowMin at bind time, before building the set — same place and order as here. It cannot move to the evaluators: a sentinel is == and hash-equal to the boundary literal, so the set collapses first. != nulls split out into #3918 (draft).

Fokko · 2026-09-08T05:02:17Z

Yea, so, what I'm trying to say is that we should not do this:

bindInOperation filters aboveMax/belowMin at bind time, before building the set

Instead, we should preserve the values and just take them into account at the evaluators

Reviewer feedback: bind should not drop what the caller wrote. Converting a literal outside the field's range yields an AboveMax/BelowMin sentinel carrying the clamped boundary value, so keep the original literal instead of the sentinel. The bound set then round-trips, and `value_set` leaves the value out so no evaluator sees it -- every BoundBooleanExpressionVisitor receives `value_set`, so that is the one place the range has to be taken into account. Storing the sentinel is not an option: it is `==` and hash-equal to the boundary literal it clamps to, so the set collapses and a boundary value the caller did write can be lost. The tests now assert evaluation results rather than the bound shape, which no longer folds, plus the round trip itself. Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot AI lite review requested due to automatic review settings September 7, 2026 03:25

Copilot started reviewing on behalf of jackylee-ch September 7, 2026 03:25 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Fokko requested changes Sep 7, 2026

View reviewed changes

jackylee-ch and others added 2 commits September 7, 2026 16:01

Navigation Menu

Uh oh!

Reject out-of-range literals when binding IN / NOT IN - #3916

Reject out-of-range literals when binding IN / NOT IN#3916
jackylee-ch wants to merge 5 commits into
apache:mainfrom
jackylee-ch:reject-out-of-range-in-literals

jackylee-ch commented Sep 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Fokko left a comment

Uh oh!

jackylee-ch commented Sep 7, 2026

Uh oh!

Fokko commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jackylee-ch commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

jackylee-ch commented Sep 7, 2026

Uh oh!

Fokko commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jackylee-ch commented Sep 7, 2026 •

edited

Loading